Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Chore: Validate Content plugin settings 2 #530

Merged
merged 1 commit into from
May 20, 2024
Merged

Chore: Validate Content plugin settings 2 #530

merged 1 commit into from
May 20, 2024
+8 −8

Conversation

iLLiCiTiT
Copy link
Member

Changelog Description

Rename ValidateContainers to ValidateOutdatedContainers.

@ynbot ynbot added the size/XS label May 20, 2024
@ynbot ynbot added the type: enhancement Improvement of existing functionality or minor addition label May 20, 2024
@iLLiCiTiT iLLiCiTiT changed the title Chore: Validate Content plugin settings Chore: Validate Content plugin settings 2 May 20, 2024
@iLLiCiTiT iLLiCiTiT merged commit 92fa97a into develop May 20, 2024
9 checks passed
@iLLiCiTiT iLLiCiTiT deleted the enhancement/better-class-name branch May 20, 2024 13:43
BigRoy
BigRoy reviewed May 20, 2024
View reviewed changes
server/settings/publish_plugins.py
ValidateContainers: ValidateContainersModel = SettingsField(
default_factory=ValidateContainersModel,
ValidateOutdatedContainers: ValidateOutdatedContainersModel = SettingsField(
default_factory=ValidateOutdatedContainersModel,
title="Validate Containers"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha - of all the changes I actually mentioned in the other PR I really meant the title to be updated since that's what the user will see in the settings directly.

Sorry, but the PR is already merged. But can we change the settings title to Validate Outdated Containers?

        title="Validate Outdated Containers"

MustafaJafar
MustafaJafar approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works.
just a small question.

server/settings/publish_plugins.py
ValidateContainers: ValidateContainersModel = SettingsField(
default_factory=ValidateContainersModel,
ValidateOutdatedContainers: ValidateOutdatedContainersModel = SettingsField(
default_factory=ValidateOutdatedContainersModel,
title="Validate Containers"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the label should be updated.

Suggested change
title="Validate Containers"
title="Validate Outdated Containers"

@dee-ynput
Copy link

Do we have a reason to not use "Validate Outdated Containers" for the title @iLLiCiTiT ?

@iLLiCiTiT
Copy link
Member Author

Do we have a reason to not use "Validate Outdated Containers" for the title @iLLiCiTiT ?

No, the suggestion was added after merge.

@dee-ynput
Copy link

dee-ynput commented Jun 5, 2024 •
edited
Loading

Do we have a reason to not use "Validate Outdated Containers" for the title @iLLiCiTiT ?

No, the suggestion was added after merge.

Yep.
So, if I'm following correctly, this second PR was to add the changes suggested after the first merge. And now is time for a third one to actually include comment about the title.

I guess my question is: "who's going to make that 3rd PR ?"

  • Roy - He asked for it... too late, two times in a row.
  • Mustafa - He asked for it... last.
  • You.

Sorry for passive agressive sarcasm... -_____-

@BigRoy
Copy link
Collaborator

BigRoy commented Jun 5, 2024 •
edited
Loading

That'll be me.

Confusing because I had to check both the Me box and the Roy box - hehe.

@BigRoy BigRoy mentioned this pull request Jun 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BigRoy BigRoy BigRoy left review comments

@MustafaJafar MustafaJafar MustafaJafar approved these changes

@antirotor antirotor antirotor approved these changes

Assignees

@antirotor antirotor

Labels
size/XS type: enhancement Improvement of existing functionality or minor addition
Projects
PR reviewing
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@iLLiCiTiT @dee-ynput @BigRoy @MustafaJafar @antirotor @ynbot